Use new Enso Hash Codes and Comparable #6060

jdunkerley · 2023-03-23T11:21:38Z

Pull Request Description

Enables distinct, aggregate and cross_tab to use the Enso hashing and equality operations.
Also, I rewired the way the ObjectComparators are obtained in polyglot code to be more consistent.

Add Comparator for Day_Of_Week, Header, SQL_Type, Image and Matrix.
Also, removed the custom == from these types as needed. (Closes #5626)

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

…Key.

…APIs.

JaroslavTulach

Few comments, but I need to understand the changes more...

JaroslavTulach · 2023-03-23T18:56:40Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Time/Day_Of_Week.enso

@@ -49,3 +53,14 @@ type Day_Of_Week
        Day_Of_Week.Thursday -> DayOfWeek.THURSDAY
        Day_Of_Week.Friday -> DayOfWeek.FRIDAY
        Day_Of_Week.Saturday -> DayOfWeek.SATURDAY
+
+## PRIVATE
+type Day_Of_Week_Comparator


This looks exactly as I have envisioned that when dreaming about Comparator design.

distribution/lib/Standard/Base/0.0.0-dev/src/Network/HTTP/Header.enso

distribution/lib/Standard/Database/0.0.0-dev/src/Data/SQL_Type.enso

JaroslavTulach · 2023-03-23T19:03:41Z

distribution/lib/Standard/Image/0.0.0-dev/src/Data/Image.enso

+        if Java_Image.is_equals x.opencv_mat y.opencv_mat then Ordering.Equal else
+            Nothing
+
+    hash x = x.opencv_mat.hashCode


Good. Delegating to Java hashCode when one knows there is a Java object is good.

JaroslavTulach · 2023-03-23T19:07:55Z

std-bits/base/src/main/java/org/enso/base/polyglot/EnsoObjectWrapper.java

+import java.util.function.BiFunction;
+import java.util.function.Function;
+
+public class EnsoObjectWrapper implements Comparable<EnsoObjectWrapper> {


Make the class final. Anyway, I am not yet sure what EnsoObjectWrapper is good for...

Let's me put Enso objects into the MulitValueIndex and hence aggregate and distinct on them using the Enso hash codes and equality.

When @Akirathan has time to do a nicer integration can switch to it but now let's all the stuff work.

Right. #5855 was a good start for the integration, I believe.

JaroslavTulach · 2023-03-23T19:20:05Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Ordering/Comparator.enso

-   - custom_comparator:
-     If `Nothing` will get an ordered comparator from each element.
-     Otherwise can support a custom fallback compare function.
-new : Nothing | (Any -> Any -> Ordering) -> ObjectComparator


Removal of this class is probably good.

That is good indeed. I was always confused by the name.

std-bits/base/src/main/java/org/enso/base/ObjectComparator.java

JaroslavTulach · 2023-03-23T19:29:43Z

std-bits/base/src/main/java/org/enso/base/ObjectComparator.java

+  private static void initCallbacks() {
+    if (EnsoCompareCallback == null) {
+      var module = Context.getCurrent().getBindings("enso").invokeMember("get_module", "Standard.Base.Data.Ordering");
+      var type = module.invokeMember("get_type", "Comparable");


This kind of poking around in Enso doesn't make me particularly happy.

However the only alternative I can offer is to always make sure each call from Enso to Java (that needs this callback) passes the compare_callback function to Java.

I also don't like such a poking in Enso. But I guess it is OK for this PR, as a temporary solution.

Akirathan

Looks good overall. I am glad that we got rid of Standard.Base.Data.Ordering.Comparator, as that name always confused me. Great that we will have the ability to aggregate over enso objects in columns, although the performance won't be good, but that we be tackled in time.

std-bits/base/src/main/java/org/enso/base/ObjectComparator.java

std-bits/base/src/main/java/org/enso/base/polyglot/EnsoObjectWrapper.java

Akirathan · 2023-03-24T09:36:56Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Ordering/Comparator.enso

-   - custom_comparator:
-     If `Nothing` will get an ordered comparator from each element.
-     Otherwise can support a custom fallback compare function.
-new : Nothing | (Any -> Any -> Ordering) -> ObjectComparator


That is good indeed. I was always confused by the name.

Akirathan · 2023-03-24T09:47:07Z

Also, if you could please add, at least a very simple, benchmark to test/Benchmarks/src/Table/Aggregate.enso that aggregates over some enso objects in the columns, that would be very cool. I could do that myself in the future, but you are definitely more versed in the API of Table.

radeusgd

Looks good as a workaround to get Enso objects comparisons/hashing supported in the Table library, although it's not what I'd imagine for the longer-term solution.

The logic for folding primitives (added to be consistent with how Enso compares them) is duplicated between the engine and our Table utilities - which may lead to inconsistency - but it does not have to be anymore as we can have this logic unified in common-polyglot-core-utils. I hope #5259 will be implemented in the not-very-far future to ensure this is consistent.

radeusgd · 2023-03-24T09:00:17Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Time/Day_Of_Week.enso

+        shifted = case first_day of
+            Day_Of_Week.Sunday -> day_number
+            _ -> (day_number + 7 - (first_day.to_integer start_at_zero=True)) % 7


Suggested change

shifted = case first_day of

Day_Of_Week.Sunday -> day_number

_ -> (day_number + 7 - (first_day.to_integer start_at_zero=True)) % 7

shifted = (day_number + 7 - (first_day.to_integer start_at_zero=True)) % 7

btw. won't just that work? I'm not sure if we have to special-case the Sunday given that we do % later anyway.

stack overflow - to_integer would be called each time,

I believe the culprit is #6065.

Oh right! Okay then let's keep it.

I guess it would have worked if we did:

day_number day = case day of Day_Of_Week.Sunday -> 0 Day_Of_Week.Monday -> 1 Day_Of_Week.Tuesday -> 2 Day_Of_Week.Wednesday -> 3 Day_Of_Week.Thursday -> 4 Day_Of_Week.Friday -> 5 Day_Of_Week.Saturday -> 6 shifted = ((day_number self) + 7 - (day_number first_day)) % 7

right?

Yes. This was only changed to avoid having == in it.

Yeah, I was just wondering on a side if we could further simplify it, removing the possibility of the infinite loop altogether.

radeusgd · 2023-03-24T09:21:34Z

distribution/lib/Standard/Table/0.0.0-dev/src/Data/Column.enso

+            wrapped a b = case a of
+                Nothing -> if b.is_nothing then Ordering.Equal else if missing_last then Ordering.Greater else Ordering.Less
+                _ -> case b of
+                    Nothing -> if missing_last then Ordering.Less else Ordering.Greater
+                    _ -> by a b


This feels like it could be a separate method that will be useful in other places:
make_comparator_nullsafe missing_last by = a-> b-> ...

radeusgd · 2023-03-24T09:29:32Z

std-bits/base/src/main/java/org/enso/base/ObjectComparator.java

-  public int compare(Object thisValue, Object thatValue) throws ClassCastException {
+  public int compare(Object thisValue, Object thatValue) {
    // NULLs
    if (thisValue == null) {
      if (thatValue != null) {


Ideally, this should also be removed and delegate to some common method in common-polyglot-core-utils. But ofc ok as a workaround, just - I assume this will be implemented by @Akirathan in #5259 (although I see that this is not scheduled nor assigned to him - do we plan to do this anytime soon @jdunkerley @JaroslavTulach?).

The code delegates to EnsoObjectWrapper and that class is using org.graalvm.polyglot.Context. We cannot/shouldn't use org.graalvm.polyglot.Context in the engine - e.g. I am afraid that isn't much to share in current implementation.

I don't understand?

This is just a workaround that should be replaced with a proper solution. The workaround uses a Context, because it is its way to 'access' the current implementation and delegate to it (through a slow Java-to-Enso call).

The whole idea of extracting the shared code is so that the logic for computing the hash for particular primitive types can be exposed in pure Java, so that our library could call into it directly in Java, without the overhead of an Java-to-Enso call. That is what is described in #5259, although maybe I did not describe this well enough. If it's not clear - let's discuss.

std-bits/table/src/main/java/org/enso/table/data/table/Column.java

radeusgd · 2023-03-24T09:41:35Z

std-bits/table/src/main/java/org/enso/table/data/table/join/scan/MatcherFactory.java

      // We could do a fast-path for some known primitive types, but it doesn't matter as it will be
      // replaced with hashing soon anyway.
-      return equalityFallback.apply(leftValue, rightValue);
+      return new EnsoObjectWrapper(leftValue).equals(new EnsoObjectWrapper(rightValue));


Another place that will need to be addressed by #5259 - especially as here we are lacking the fast-path for primitives.

Maybe we could move foldObject from MultiValueKey to something next to ObjectComparator and then delegate to such helper for equality? Then we would get the fast-path for primitives, which is pretty important.

Although I'm fine if we leave this as-is for now and handle the proper solution in #5259.

radeusgd · 2023-03-24T09:44:06Z

test/Table_Tests/src/In_Memory/Table_Spec.enso

-        Test.specify "until hashing is supported, should throw an error when trying to aggregate a custom object" <|
+        Test.specify "should be able to create distinct on Enso objects" <|
            t = Table.new [["X", [My.Data 1 2, My.Data 3 4, My.Data 1 2]]]
-            t.distinct . should_fail_with Illegal_Argument
+            t.distinct ["X"] . at "X" . to_vector . should_equal [My.Data 1 2, My.Data 3 4]
+
+            t2 = Table.new [["X", [Day_Of_Week.Monday, Day_Of_Week.Tuesday, Day_Of_Week.Monday, Day_Of_Week.Monday, Day_Of_Week.Tuesday, Day_Of_Week.Wednesday]]]
+            t2.distinct ["X"] . at "X" . to_vector . should_equal [Day_Of_Week.Monday, Day_Of_Week.Tuesday, Day_Of_Week.Wednesday]


I think we should add tests like this to Table.join too.

Also, ideally would also test some My_Atom that has a custom comparator defined.

You can just use My type - there is custom_objects column here, tested on line 844.

std-bits/table/src/main/java/org/enso/table/data/table/join/scan/MatcherFactory.java

* develop: Layout fixes (#6066) Use new Enso Hash Codes and Comparable (#6060) Search suggestions by static attribute (#6036) Use .node-version for pinning Node.js version (#6057) Fix code generation for suggestion preview (#6054) Implementation of EnsoGL predefined Rectangle shape. (#6033) Tidy up the public module level statics (#6032) Cursor aware Component Browser (#5770)

jdunkerley linked an issue Mar 23, 2023 that may be closed by this pull request

Investigate the possibility of passing through the Enso Map to UnorderedMultiValueIndex #5046

Closed

jdunkerley added 4 commits March 23, 2023 15:30

Looking at passing Value along.

491a988

WIP.

3840517

Use the Ordering functions for Enso values in the UnorderedMultiValue…

314b152

…Key.

Work in progress to have single ObjectComparator calling back to new …

410cbdc

…APIs.

jdunkerley force-pushed the wip/jd/5046-investigate-the-possibility-of-passing-through-the-enso-map-to-unorderedmultivalueindex branch from 90b5d40 to 410cbdc Compare March 23, 2023 16:19

jdunkerley added 4 commits March 23, 2023 16:19

Remove legacy file.

90489c9

Java format and fix failing tests.

9dc5ae0

Fix typo.

ddba647

Remove Comparator.enso.

bf622c3

jdunkerley added the CI: No changelog needed Do not require a changelog entry for this PR. label Mar 23, 2023

Fix imports.

8b7e895

jdunkerley requested a review from GregoryTravis March 23, 2023 17:37

jdunkerley added 5 commits March 23, 2023 17:39

Sort out a couple of issues.

53e3471

Use Ordering.compare.

7ff0f48

Remove all custom == (except Column).

20abf27

Missing import.

dfbae3b

Incorrect imports.

0b1402f

jdunkerley marked this pull request as ready for review March 23, 2023 18:13

jdunkerley requested a review from radeusgd as a code owner March 23, 2023 18:13

jdunkerley requested review from Frizi and JaroslavTulach March 23, 2023 18:18

GregoryTravis approved these changes Mar 23, 2023

View reviewed changes

JaroslavTulach reviewed Mar 23, 2023

View reviewed changes

jdunkerley requested review from Akirathan and removed request for Frizi March 23, 2023 19:20

JaroslavTulach approved these changes Mar 23, 2023

View reviewed changes

enso-bot bot mentioned this pull request Mar 24, 2023

Investigate More Effective IR Caching format #5567

Closed

PR comments.

4c975ea

Akirathan approved these changes Mar 24, 2023

View reviewed changes

radeusgd approved these changes Mar 24, 2023

View reviewed changes

jdunkerley added 3 commits March 24, 2023 11:25

Move logic into single place.

55c2b3a

Add aggregate test on Day_Of_Week.

52b6aac

Add order by test.

45af32b

jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Mar 24, 2023

mergify bot merged commit 58f2c76 into develop Mar 24, 2023

mergify bot deleted the wip/jd/5046-investigate-the-possibility-of-passing-through-the-enso-map-to-unorderedmultivalueindex branch March 24, 2023 15:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use new Enso Hash Codes and Comparable #6060

Use new Enso Hash Codes and Comparable #6060

jdunkerley commented Mar 23, 2023 •

edited

Loading

JaroslavTulach left a comment

JaroslavTulach Mar 23, 2023

JaroslavTulach Mar 23, 2023

JaroslavTulach Mar 23, 2023

jdunkerley Mar 23, 2023

Akirathan Mar 24, 2023

JaroslavTulach Mar 23, 2023

Akirathan Mar 24, 2023

JaroslavTulach Mar 23, 2023

Akirathan Mar 24, 2023

Akirathan left a comment

Akirathan Mar 24, 2023

Akirathan commented Mar 24, 2023

radeusgd left a comment •

edited

Loading

radeusgd Mar 24, 2023

jdunkerley Mar 24, 2023

Akirathan Mar 24, 2023

radeusgd Mar 24, 2023

jdunkerley Mar 24, 2023

radeusgd Mar 24, 2023

radeusgd Mar 24, 2023

radeusgd Mar 24, 2023

JaroslavTulach Mar 27, 2023

radeusgd Mar 27, 2023

radeusgd Mar 24, 2023

radeusgd Mar 24, 2023

radeusgd Mar 24, 2023

Akirathan Mar 24, 2023

Use new Enso Hash Codes and Comparable #6060

Use new Enso Hash Codes and Comparable #6060

Conversation

jdunkerley commented Mar 23, 2023 • edited Loading

Pull Request Description

Checklist

JaroslavTulach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akirathan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akirathan commented Mar 24, 2023

radeusgd left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdunkerley commented Mar 23, 2023 •

edited

Loading

radeusgd left a comment •

edited

Loading